t2m v2 (epic #837): twist transport, foot pinning, flow-matching model#904
Conversation
The bind-referenced direction retarget aimed each bone but inherited the target bind's roll — gesture clips read flat and the roll component between mismatched decomposition poles leaked into swing (arm amplitude +38% on Mixamo self-retarget). Now the source's swing/twist split is recomposed about the SAME pole on the target: Qbase = arc(dt_bind -> d_ref) * Wt_bind (once per bone) Wt(f) = twist(theta*gain, ds) * arc(d_ref -> ds) * Qbase theta is the source's signed roll about its reference bone direction, wrapped to (-pi,pi] and unwrapped across frames (a +-180deg pop would flip the roll once a gain != 1 scales it), capped at 150deg; collars are damped 0.5x (they share the shoulder line's roll). Direction tracking stays absolute, so cross-rig robustness is unchanged. Mixamo Walk self-parity: 2.22deg -> 0.03deg mean joint delta (target was <=1.6deg); spine amplitude restored (abdomen 2.3deg -> 9.0deg amp), legs exact; Rumba walk/dance/wave and Quaternius Knight renders upright with natural hanging arms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pler, reference triple (#840, #858) Data (#858): scripts/prep-t2m-v5.py canonicalizes every clip (corpus *.canonical.json dumps + CMU BVH) into a rig-independent representation before windowing — per joint, aim-from-canonical-T-pose composed with the swing/twist residual: Q'(f) = twist(theta, d(f)) * arc(D_c -> d(f)). Identical for every rig performing the same motion, and exactly what the bind-referenced retarget consumes given the canonical reference triple (restWorld = identity, restDir = D_c). 17,286 windows / 18 actions (corpus 517 + CMU 16,789), replacing the v4 per-rig convention mush. Model (#840): scripts/train-t2m-flow-v5.py — small DiT-style velocity transformer (7.3M params, 6D rotations, AdaLN-Zero conditioning) trained with the rectified-flow objective + classifier-free guidance; flow matching samples a transport path to a single data mode instead of decoding a conditional mean (the CVAE's structural weakness). The Euler sampler (+CFG) is UNROLLED INSIDE the exported ONNX graph, so the shipped MotionGenerator contract (tokens[1,V], seed[1,Z] -> motion [1,T,220]) is unchanged — seed is the flattened noise tensor. Wiring (#858): MotionGenerator parses the vocab's restWorld/restDir into the generated clip; CLI/MCP/GUI model paths pass it to applyMotionClip, so model clips ride the SAME bind-referenced direction retarget as v5 template clips — the synthetic-standing-pose shim remains only as the legacy-v4 fallback. applyMotionClip now treats any valid unit-norm reference quat as present (identity counts; only zero-filled entries mean absent), which the v5 identity triple requires. Render-verified on the Rumba rig: model walk/run/wave/dance all upright and temporally coherent end-to-end (the v4 CVAE folded or tumbled); templates remain the default + automatic fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Retargeted clips slide/float feet on rigs whose proportions differ from the source — the direction retarget transfers bone directions, not world foot positions. Core (FootContact.h/cpp, Ogre-free + unit-tested): contact detection (foot within a leg-length-scaled height band of the clip's ground level AND nearly stationary horizontally -> spans; ground = low percentile so jump clips don't skew it), analytic two-bone IK (solveKnee — keeps both segment lengths, preserves the pose's own bend plane, clamps unreachable targets), and the 0->1->0 edge blend weight. AnimationMerger::pinFeet: manual FK over the tracks (pure track math — the live skeleton is never apply()'d), detection in the canonical frame (Ct) so ground is horizontal regardless of rig axes, then per contact frame locks the foot to its span-start position blended over blendFrames: thigh re-aimed at the IK knee, shin at the pinned target, foot keyframe compensated to keep its ORIGINAL world orientation (no toe pop). Only thigh/shin/foot keyframes rewritten; _keyFrameDataChanged on each (the #854 cache gotcha). Effectively idempotent (re-detection re-pins to the same targets); 'qtme.footpin.<anim>' marker on bone[0], cleared when applyMotionClip regenerates the clip. Surfaces: ON by default in generation — CLI --no-foot-pin opts out, standalone 'qtmesh anim <f> --foot-pin --animation <name> -o out'; MCP foot_pin arg on generate_motion + standalone pin_feet tool; GUI 'Pin feet (contact cleanup)' checkbox in the Animations section. Also: canonicalIndexForBone learns 'upperleg' as a thigh alias — Quaternius-style rigs (UpperLeg.L/LowerLeg.L) previously mapped BOTH leg bones to the knee role, which broke pinning and double-aimed the knee in the retarget. Verified: Rumba walk 2 contact spans pinned; Knight (UpperLeg/LowerLeg naming) pins after the alias fix; renders upright, stride intact. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ectation (#857) The arm-rig helper resolves only 9/22 roles, below applyMotionClip's >=11 humanoid gate — the roll test now builds a 13-role rig (+hands, +feet). And the 150-degree runaway-unwrap cap applies BEFORE the collar gain, so a 240-degree source roll lands at 150 x 0.5 = 75 degrees, not 120; the continuity assertion (the actual unwrap regression check) is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds v5 canonical motion preparation and flow-matching export, propagates canonical rest metadata through retargeting, introduces foot-contact detection and IK pinning, and exposes cleanup through QML, CLI, MCP, and generated-motion workflows. ChangesMotion generation pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant QML
participant AnimationControlController
participant MotionGenerator
participant AnimationMerger
QML->>AnimationControlController: generateMotion(prompt, footPin)
AnimationControlController->>MotionGenerator: generate canonical motion
MotionGenerator-->>AnimationControlController: clip with restWorld/restDir
AnimationControlController->>AnimationMerger: smooth bake and pin feet
AnimationMerger-->>AnimationControlController: pinned spans and keyframes
AnimationControlController-->>QML: generation result and footPinSpans
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: faa7f4f4e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!thighTrk || !shinTrk | ||
| || static_cast<int>(thighTrk->getNumKeyFrames()) != nk | ||
| || static_cast<int>(shinTrk->getNumKeyFrames()) != nk | ||
| || (footTrk && static_cast<int>(footTrk->getNumKeyFrames()) != nk)) | ||
| continue; // mixed keyframe grids — not a generated clip |
There was a problem hiding this comment.
Compare leg keyframe times before pinning
When --foot-pin/pin_feet is run on an authored animation where the thigh, shin, and foot tracks have the same number of keys but not the same key times, this count-only guard lets the post-process proceed as if the grids match. The FK samples are taken at timeSrc times, but the rewrite later uses getNodeKeyFrame(k) on the shin/foot tracks, so corrections computed for one timestamp can be written to a different timestamp and corrupt the leg pose; compare the actual keyframe times or resample/create keys before editing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
src/AnimationMerger.cpp (2)
1803-1809: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
footPinKey()helper instead of a hardcoded literal.
footPinKey(animName)is defined right above (Line 1475-1478) specifically to produce this key, and the siblingarmSpaceKey(animName)call on the previous line correctly uses its helper — but this line re-types the literal"qtme.footpin." + animName"instead of callingfootPinKey(animName). If the key format ever changes, only one of the two call sites (the setter inpinFeet, Line 1697-1698) would get updated, silently breaking the erase-on-regenerate behavior.♻️ Proposed fix
// `#856`: same for the foot-pin marker — a regenerated clip is unpinned. skel->getBone(0)->getUserObjectBindings().eraseUserAny( - "qtme.footpin." + animName); + footPinKey(animName));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/AnimationMerger.cpp` around lines 1803 - 1809, Replace the hardcoded foot-pin key construction in the regeneration cleanup block with the existing footPinKey(animName) helper. Keep the surrounding getUserObjectBindings().eraseUserAny calls and armSpaceKey(animName) behavior unchanged.
1481-1701: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winpinFeet math checks out; consider hardening the mixed-grid guard.
The FK reconstruction, two-bone IK correction (S1/S2), and keyframe-formula composition are consistent with the
bindLocal⁻¹·Wp⁻¹·Wnewconvention used elsewhere in this file (e.g.adjustArmSpace). One spot worth tightening: the guard at Line 1588-1592 only compares keyframe counts across thigh/shin/foot tracks to detect "mixed keyframe grids," which is a proxy for "same time grid." It's correct for the primary target (generated clips, all tracks sharing anf*dtgrid), but two tracks could coincidentally share a count while differing in actual keyframe times, silently mis-aligning the rewrite. Comparing the actual time arrays (not just counts) would close that gap cheaply.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/AnimationMerger.cpp` around lines 1481 - 1701, The mixed-grid validation in pinFeet currently checks only keyframe counts; update it to compare each thigh, shin, and optional foot track’s actual keyframe times against the reference times array before processing the leg. Skip the leg when any track’s timestamps differ, while preserving the existing count checks and generated-clip behavior.src/MCPServer.cpp (2)
4272-4341: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
file.exportbreadcrumb for the re-export step.
toolPinFeetadds anai.tool_callbreadcrumb for the tool invocation, but the export viaMeshImporterExporter::exporterat line 4319 has no accompanying breadcrumb. As per coding guidelines,"Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb; ... file.import/file.export for I/O."— the export write should get its ownfile.exportbreadcrumb for observability parity with other I/O-performing tools.🔧 Proposed fix
const QString outPath = args.value("output_path").toString(); if (!outPath.isEmpty()) { + SentryReporter::addBreadcrumb(QStringLiteral("file.export"), + QStringLiteral("MCP pin_feet export: %1").arg(outPath)); auto* node = entity->getParentSceneNode(); if (MeshImporterExporter::exporter( node, outPath, CLIPipeline::formatForExtension(outPath)) != 0)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/MCPServer.cpp` around lines 4272 - 4341, Add a SentryReporter::addBreadcrumb call immediately before MeshImporterExporter::exporter in MCPServer::toolPinFeet, using the file.export category and including the output path or equivalent export context. Keep the existing export failure handling and result behavior unchanged.Source: Coding guidelines
4144-4150: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFoot-pin failure is silently swallowed in
generate_motion.When
AnimationMerger::pinFeetfails,fp.erroris discarded andfootPinSpansjust falls back to-1— identical to the "disabled" state. The caller has no way to distinguish "opted out" from "ran and failed" for debugging, unliketoolPinFeetwhich surfacesfp.errordirectly.🛡️ Proposed fix to surface the failure reason
int footPinSpans = -1; + QString footPinError; if (args.value("foot_pin").toBool(true)) { const auto fp = AnimationMerger::pinFeet(skel.get(), animName); - footPinSpans = fp.ok ? fp.spans : -1; + footPinSpans = fp.ok ? fp.spans : -1; + if (!fp.ok) footPinError = fp.error; }And later:
if (footPinSpans >= 0) content["foot_pin_spans"] = footPinSpans; + if (!footPinError.isEmpty()) content["foot_pin_error"] = footPinError;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/MCPServer.cpp` around lines 4144 - 4150, Update the foot-pinning block in generate_motion around AnimationMerger::pinFeet so failed pinFeet results surface fp.error to the caller, matching toolPinFeet behavior. Preserve footPinSpans == -1 for the explicit foot_pin:false opt-out, but do not silently treat an enabled-operation failure as disabled.qml/PropertiesPanel.qml (1)
8539-8561: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer the existing
InspectorCheckBoxcomponent over a raw Rectangle checkbox.
footPinChkduplicates the ad-hoc Rectangle+MouseArea checkbox pattern ofuseModelChkright above it, instead of the file's own themedInspectorCheckBoxcomponent (lines 25-64), which already provides keyboard focus,Keys.on*Pressedactivation, andAccessible.*properties. As written, this checkbox can't be toggled via keyboard and has no accessible name — a task-completion gap for keyboard/assistive-tech users.♻️ Proposed fix
- Row { - spacing: 6 - Rectangle { - id: footPinChk - property bool checked: true - width: 14; height: 14; radius: 2 - anchors.verticalCenter: parent.verticalCenter - color: checked ? PropertiesPanelController.highlightColor - : PropertiesPanelController.inputColor - border.color: PropertiesPanelController.borderColor - Text { anchors.centerIn: parent; visible: parent.checked - text: "✓"; color: "white"; font.pixelSize: 10 } - MouseArea { anchors.fill: parent - onClicked: footPinChk.checked = !footPinChk.checked } - } - Text { - text: "Pin feet (contact cleanup)" - color: PropertiesPanelController.textColor; font.pixelSize: 10 - anchors.verticalCenter: parent.verticalCenter - } - } + InspectorCheckBox { + id: footPinChk + text: "Pin feet (contact cleanup)" + checked: true + }(Update the read site at line 8496 to
footPinChk.checked, which already works withCheckBox's built-in property.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qml/PropertiesPanel.qml` around lines 8539 - 8561, Replace the raw Rectangle and MouseArea implementation of footPinChk with the existing InspectorCheckBox component, preserving its default checked state and label behavior. Update the related read site to use footPinChk.checked, and configure the component’s accessible text/name for “Pin feet (contact cleanup)” so keyboard and assistive-technology interaction work through the shared component.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/prep-t2m-v5.py`:
- Around line 213-215: Guard the per-item processing in the clip iteration
around np.asarray/canonicalize and the BVH processing around
prep4.parse_bvh/bvh_rest with try/except so malformed clips are skipped without
aborting the batch. Catch processing exceptions, log the skipped item and
exception details, then continue; preserve successful yields and existing
JSON-loading behavior.
- Around line 331-338: Guard the vocabulary-building block after computing vocab
and keep: if no actions meet a.min_action_windows, exit with a clear error
message before calling np.stack. Preserve the existing stacking behavior when
keep is non-empty, using the surrounding script’s established sys.exit
validation style.
In `@scripts/train-t2m-flow-v5.py`:
- Line 189: Update the np.load call in the training data-loading flow to remove
allow_pickle=True and explicitly use the safe default of allow_pickle=False,
while preserving the existing archive loading behavior.
In `@scripts/upload-t2m-v5-model.sh`:
- Around line 31-43: Make the backup loop in the upload script idempotent by
checking whether each destination in the case mapping already exists in the
repository before uploading; skip that backup when present so reruns never
overwrite the preserved v4 assets. Also ensure the temporary directory created
by TMP is removed on script exit, including failure paths.
In `@src/AnimationControlController.cpp`:
- Around line 1948-1954: Update the footPin handling in the animation output
flow to surface FootPinResult::error when AnimationMerger::pinFeet returns a
failure. Preserve the existing footPinSpans output for successful results with
spans, and expose the error through the established output/reporting mechanism
so users can distinguish failure from finding no contacts.
In `@src/AnimationMerger_test.cpp`:
- Around line 1310-1336: Update the stale introductory comment in the dense
sampling test around anim and maxStepDeg to state that the collar should move
continuously and end near the capped 75° result about +X. Keep it consistent
with the existing EXPECT_NEAR assertion and the subsequent 150° cap followed by
0.5× gain explanation.
- Around line 1263-1291: Add an explicit non-null assertion for the entity
returned by createEntity in TwistUnwrapKeepsDampedCollarContinuous before
calling ent->getSkeleton(), matching the existing check in
TwistTransportCarriesBoneRoll.
- Around line 1181-1185: Update twistDegAbout to avoid unguarded M_PI usage by
switching its degree conversion to Ogre::Math::PI, or add the same local
_USE_MATH_DEFINES/MSVC-safe PI fallback used elsewhere in the test suite.
---
Nitpick comments:
In `@qml/PropertiesPanel.qml`:
- Around line 8539-8561: Replace the raw Rectangle and MouseArea implementation
of footPinChk with the existing InspectorCheckBox component, preserving its
default checked state and label behavior. Update the related read site to use
footPinChk.checked, and configure the component’s accessible text/name for “Pin
feet (contact cleanup)” so keyboard and assistive-technology interaction work
through the shared component.
In `@src/AnimationMerger.cpp`:
- Around line 1803-1809: Replace the hardcoded foot-pin key construction in the
regeneration cleanup block with the existing footPinKey(animName) helper. Keep
the surrounding getUserObjectBindings().eraseUserAny calls and
armSpaceKey(animName) behavior unchanged.
- Around line 1481-1701: The mixed-grid validation in pinFeet currently checks
only keyframe counts; update it to compare each thigh, shin, and optional foot
track’s actual keyframe times against the reference times array before
processing the leg. Skip the leg when any track’s timestamps differ, while
preserving the existing count checks and generated-clip behavior.
In `@src/MCPServer.cpp`:
- Around line 4272-4341: Add a SentryReporter::addBreadcrumb call immediately
before MeshImporterExporter::exporter in MCPServer::toolPinFeet, using the
file.export category and including the output path or equivalent export context.
Keep the existing export failure handling and result behavior unchanged.
- Around line 4144-4150: Update the foot-pinning block in generate_motion around
AnimationMerger::pinFeet so failed pinFeet results surface fp.error to the
caller, matching toolPinFeet behavior. Preserve footPinSpans == -1 for the
explicit foot_pin:false opt-out, but do not silently treat an enabled-operation
failure as disabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5f4b819-b876-4488-886f-997ef155bfac
📒 Files selected for processing (21)
qml/PropertiesPanel.qmlscripts/prep-t2m-v5.pyscripts/train-t2m-flow-v5.pyscripts/upload-t2m-v5-model.shsrc/AnimationControlController.cppsrc/AnimationControlController.hsrc/AnimationMerger.cppsrc/AnimationMerger.hsrc/AnimationMerger_test.cppsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CMakeLists.txtsrc/FootContact.cppsrc/FootContact.hsrc/FootContact_test.cppsrc/MCPServer.cppsrc/MCPServer.hsrc/MotionGenerator.cppsrc/MotionGenerator.hsrc/MotionInbetween.cpptests/CMakeLists.txt
| ap.add_argument("--device", default="mps") | ||
| a = ap.parse_args() | ||
|
|
||
| d = np.load(a.data, allow_pickle=True) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the npz produced by prep-t2m-v5.py never stores object-dtype arrays.
python3 - <<'EOF'
import numpy as np
vocab = ["walk", "run", "idle"]
a = np.array(vocab)
print("vocab dtype:", a.dtype, "is object:", a.dtype == object)
EOFRepository: fernandotonon/QtMeshEditor
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the producer and inspect the relevant code paths.
git ls-files | rg 'prep-t2m-v5\.py|train-t2m-flow-v5\.py|t2m|np\.savez|np\.load'
echo '--- outline: scripts/prep-t2m-v5.py'
ast-grep outline scripts/prep-t2m-v5.py --view expanded || true
echo '--- outline: scripts/train-t2m-flow-v5.py'
ast-grep outline scripts/train-t2m-flow-v5.py --view expanded || true
echo '--- relevant snippets from producer/consumer'
rg -n -C 3 'savez|save\(|load\(|allow_pickle|vocab|object' scripts/prep-t2m-v5.py scripts/train-t2m-flow-v5.py || trueRepository: fernandotonon/QtMeshEditor
Length of output: 9331
Drop allow_pickle=True here. prep-t2m-v5.py only writes numeric arrays plus a Unicode vocab array, so np.load(..., allow_pickle=False) is sufficient and avoids opening a pickle-based code-execution path if the archive is ever replaced.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/train-t2m-flow-v5.py` at line 189, Update the np.load call in the
training data-loading flow to remove allow_pickle=True and explicitly use the
safe default of allow_pickle=False, while preserving the existing archive
loading behavior.
Source: Linters/SAST tools
| float twistDegAbout(const Ogre::Quaternion& q, const Ogre::Vector3& ax) | ||
| { | ||
| const float s = q.x * ax.x + q.y * ax.y + q.z * ax.z; | ||
| return 2.0f * std::atan2(s, q.w) * 180.0f / static_cast<float>(M_PI); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether M_PI is used elsewhere and whether _USE_MATH_DEFINES is set globally.
rg -n 'M_PI' --type=cpp -g '!**/build/**'
rg -n '_USE_MATH_DEFINES' CMakeLists.txt src/CMakeLists.txt 2>/dev/nullRepository: fernandotonon/QtMeshEditor
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== files ==\n'
git ls-files | sed -n '1,200p'
printf '\n== M_PI occurrences ==\n'
rg -n --hidden --glob '!**/build/**' 'M_PI|_USE_MATH_DEFINES' .
printf '\n== CMakeLists snippets ==\n'
for f in CMakeLists.txt src/CMakeLists.txt; do
if [ -f "$f" ]; then
echo "--- $f ---"
nl -ba "$f" | sed -n '1,220p'
fi
done
printf '\n== target file around snippet ==\n'
nl -ba src/AnimationMerger_test.cpp | sed -n '1160,1205p'Repository: fernandotonon/QtMeshEditor
Length of output: 12910
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== search for math constants/PI helpers ==\n'
rg -n --hidden --glob '!**/build/**' 'static_cast<float>\(M_PI\)|std::numbers::pi|M_PI_F|PI' src include .github CMakeLists.txt 2>/dev/null || true
printf '\n== includes in target file ==\n'
sed -n '1,80p' src/AnimationMerger_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 38230
Add an MSVC-safe PI fallback here
src/AnimationMerger_test.cpp:1181 uses M_PI without the local _USE_MATH_DEFINES guard used elsewhere in the test suite. Add the same fallback here or switch to Ogre::Math::PI to keep the Windows/MSVC test build portable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/AnimationMerger_test.cpp` around lines 1181 - 1185, Update twistDegAbout
to avoid unguarded M_PI usage by switching its degree conversion to
Ogre::Math::PI, or add the same local _USE_MATH_DEFINES/MSVC-safe PI fallback
used elsewhere in the test suite.
Source: Coding guidelines
…pin, twist caps+low-pass, library gates (#837) Field feedback after the epic branch: generated walks trembled, played 'too fast', barely moved the legs, and over-moved arms/head. Four root causes, four fixes: 1. FOOT PINNING OVER-DETECTED (the frozen legs): the height band was 0.18 of leg length — a walking foot only lifts ~0.05-0.15, so contact spans swallowed the swing phase and pinned the legs into a shuffle while the upper body kept full rate (which also reads as 'too fast'). Tightened to 0.06 band / 0.012 velocity / 4-frame minimum, plus a coverage guard: any single span covering >55% of the clip is a misdetection and is discarded. 2. TWIST JITTER (the trembling): theta comes from per-frame shortest-arc decompositions and jitters near degenerate aims. Added the issue's 'optional low-pass' (5-tap binomial per role) and replaced the single 150-degree cap with per-role caps (neck/head 30, spine 45, arms 90, legs 60, feet 45; hip keeps 150 for genuine facing turns). 3. SMOOTH-BAKE POST-PASS (the user's own trick, codified): AnimationMerger::smoothBakeAnimation re-grids the clip to a sparse rate then back to the clip rate — a temporal low-pass that removes residual retarget trembling. ON by default in generation, before arm-space and foot pinning so pin targets stay exact. CLI --no-smooth-bake / --smooth-fps N; MCP smooth_bake/smooth_fps; GUI default path. 4. LIBRARY TAKE GATES (the T-pose/raised arms and hidden head): several corpus takes render broken regardless of retarget quality — unresolved arm roles freeze the target's arms in its literal bind T-pose; zombie/skeleton takes hold arms horizontal; several rigs carry a per-bone axis inversion that points one arm skyward or the neck straight down (head renders thrown back). New curation gates in build-motion-library-v5.py judge each arm's SIGNED hang direction, require an upright neck chain, and drop locomotion takes with unresolved arms; CrouchWalk/CrouchRun map to a new 'crouch' action instead of polluting walk/run. Curated v6 library (87 clips, 22 actions) uploaded to HF. Render-verified: 4/4 random Rumba walks natural (arms hanging, head visible, full stride). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Quality follow-up pushed (9c54de7) after field testing found generated walks trembling, playing "too fast", with frozen legs and over-active arms/head. Four root causes:
Render-verified: 4/4 random Rumba walks natural (arms hanging, head visible, full stride). |
…llow-up) The v5 model learned a head-down hunched walk because training windows were unfiltered — folded, idle-contaminated and placeholder-armed source content trained in. In the canonical representation the checks are trivial (d(f) = Q'(f)*D_c): spine and neck/head must stay up (stricter for locomotion), and locomotion upper arms must HANG — judged on the SIGNED up-component per arm, the library-curation lesson (abs() passes a skyward arm). Dropped 841 of 17,193 windows; the retrained model (hosted as t2m v5.1) walks upright with stepping legs on both the Mixamo test rig and the chibi mouse that exposed the hunch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CFG at 2.0 on the posture-filtered dataset extrapolates past the (now upright) conditional mean into a backward torso arch — the overshoot is proportional to how far the filtered data sits from the uncond distribution. Render sweep on the Mixamo rig: g2.0 arched back, g1.3 head-back creep, g1.0 clean upright walk/run and an upright chibi-mouse walk. Guidance stays a knob for conditioning-starved future datasets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…837) Days-scale quality pass on the text-to-motion model after field feedback (hunched walk, ballerina feet, head not level). Data (scripts/prep-t2m-v6.py): apply the SAME quality gates that curate the shipping template library to every training window — upright spine and neck/head (stricter for locomotion), locomotion upper arms must hang (signed per-arm Y), sane energy band — fold the curated library takes in directly, and augment with sagittal mirror + 0.85x/1.15x speed. 58,972 windows / 21 actions (was ~17k unfiltered), T=60. Model (scripts/train-t2m-flow-v5.py): 21.7M params (dim 384, 8 layers), 400 epochs, per-epoch checkpoint + --resume so multi-day runs survive sleeps. Converged at loss 0.037 (old floor ~0.09). Inference (src/MotionGenerator.cpp): 16-candidate best-of-N ranked by posture criteria that DOMINATE the motion terms — spine/head up, head not tipped back (|forward-Z| penalty), each upper arm hanging (worst arm dominates) — so a bad draw can never win. Plus a foot-articulation guard: when the model under-moves a foot role, drop its reference dir so the rig keeps its bind foot pitch (kills the ballerina toes). Numerically on-distribution vs the training walk: spine Y 0.99 (data 0.98), head Y 0.84 (0.88), arms Y -0.93/-0.95 (data -0.87). Render- verified upright walk/run on the Mixamo rig and the chibi mouse that exposed the original hunch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
v6 model quality pass (86fbd17) — days-scale retrain addressing the field report (hunched walk, ballerina feet, head not level, chest tremble). Three fronts:
Numerically on-distribution vs the training walk (spine Y 0.99 / data 0.98; head Y 0.84 / 0.88; arms −0.93 / −0.87). Render-verified upright on the Mixamo rig + the chibi mouse. Hosted on HF; template path stays the default, model checkbox is now genuinely usable. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/MotionGenerator.cpp (1)
312-315: 🚀 Performance & Scalability | 🔵 TrivialVerify latency impact of doubling
kCandidates.
kCandidatesdoubled from 8→16, and posture scoring (lines 398-435) adds extra O(T·J) passes per candidate. Per the coding guidelines, MCP execution runs on the main thread, sogenerate()'s total blocking time is what matters for responsiveness. The in-code comment says inference is "~ms per run," so this is likely fine, but worth confirming the doubled candidate count plus new scoring loops still keeps end-to-end latency acceptable for main-thread execution.As per coding guidelines, "MCP execution runs on the main thread and blocking queued calls can deadlock" — applying that same main-thread constraint here to flag the added compute cost.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/MotionGenerator.cpp` around lines 312 - 315, Measure end-to-end generate() latency with kCandidates set to 16, including the posture-scoring passes, under main-thread execution. Compare it with the previous 8-candidate baseline and verify responsiveness remains within the project’s acceptable budget; reduce candidates or optimize the scoring path if the doubled count causes unacceptable blocking.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/train-t2m-flow-v5.py`:
- Line 231: Update the resume checkpoint load around torch.load in the training
flow to pass weights_only=True, preserving map_location=dev. If compatibility
with older PyTorch versions is required, conditionally use the safe argument
only when supported while retaining the existing resume behavior.
In `@src/MotionGenerator.cpp`:
- Around line 322-339: Validate that J equals 22 before enabling
canonical-direction processing, by incorporating this requirement into
haveCanonDirs (or asserting it when canonical directions are present). Ensure
qRotDir, the meanDir calls for roles 1, 5, 7, and 11, and the foot guard using
roles 17 and 21 cannot access vocabRestDir, w, or best with those constants
unless the validated 22-joint layout is available.
---
Nitpick comments:
In `@src/MotionGenerator.cpp`:
- Around line 312-315: Measure end-to-end generate() latency with kCandidates
set to 16, including the posture-scoring passes, under main-thread execution.
Compare it with the previous 8-candidate baseline and verify responsiveness
remains within the project’s acceptable budget; reduce candidates or optimize
the scoring path if the doubled count causes unacceptable blocking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef48b1e6-d896-43be-a31a-048707314386
📒 Files selected for processing (3)
scripts/prep-t2m-v6.pyscripts/train-t2m-flow-v5.pysrc/MotionGenerator.cpp
Field report: v6 walk motion was good but stepped SIDEWAYS, twisting the hips and deforming the mesh. Root cause traced numerically: 59% of the raw CMU walk windows are themselves splayed/side-stepping (foot fwd/side travel < 1.5), and the model faithfully learned that majority — the v6 model's foot fwd/side ratio was 1.2 vs a clean stride's 3.3. Fix: a stride-directionality gate in prep-t2m-v6.py — forward-kinematics each window over the canonical bone directions and keep only clips whose feet travel forward >= 2x sideways. Drops the bad 59%, leaving 7,724 clean forward-stride walk windows (plenty). Retrained v6.1 (same 21.7M / 400ep setup): model foot fwd/side ratio 1.2 -> 2.29, posture held (spine 0.99, head 0.90, arms -0.98). Render-verified clean forward walk on the Mixamo rig and the chibi mouse that exposed the bug. Lesson: measure motion metrics NUMERICALLY via FK against the training distribution — the isometric sprite sheets foreshorten posture and hid both this and the earlier 'hunch' false alarm. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
v6.1 — sideways-walk fix (66ac50f), shipped. Field report after v6: the walk motion was finally good, but the character stepped sideways (feet splaying out), twisting the hips and deforming the mesh. Root cause (traced numerically, not by eye): 59% of the raw CMU walk windows are themselves splayed/side-stepping — foot forward/side travel < 1.5 — and the model faithfully learned that majority. Measured: v6 model foot fwd/side ratio 1.2 vs a clean stride's 3.3. Fix: a stride-directionality gate in
Render-verified: clean forward walk on the Mixamo rig and the chibi mouse that exposed the bug. Hosted on HF; local cache refreshed. Template path stays default; the model checkbox is now genuinely usable. Lesson: measure motion metrics numerically via FK against the training distribution — the isometric sprite sheets foreshorten posture and masked both this and an earlier 'hunch' false alarm. |
Field report: the v6.1 model walk was good but (1) faced/travelled
BACKWARD on the target rig, (2) pointed the toes (ballerina feet), and
(3) hung the arms too narrow (crossing behind). All three are retarget-
layer artifacts of the model's canonical convention, fixed in data-space
so the delicate bind-referenced aim math is untouched:
AnimationMerger::conditionModelClip (model clips only; templates are
authored and skip it):
(1) FACING — the model's canonical clip faces -Z, so it needs a 180deg
yaw about canonical +Y to match the +Z-forward retarget. Applied to
the clip quats (directions just negate in X/Z, so getRotationTo
stays well-conditioned — the aim path itself can't apply yaw180
without sending aims near-anti-parallel to their bind dirs). The
per-rig backward heuristic XORs with it (!yaw180): a backward rig
cancels the model flip. Verified: front of body/face now toward
camera on both the Mixamo rig and the chibi mouse.
(2) ARMS — model hangs them too narrow (measured lateral 0.0-0.08 vs
training 0.15); widen the upper-arm world quat ~12deg outward about
canonical forward, mirrored per side, inherited by elbow/hand.
(3) FEET — model under-pitches the feet (toe-Y -0.17 vs training -0.42)
and the aim then points the toes; zero the foot restDir so the rig
keeps its bind foot pitch (unconditional for model clips — the
model's foot signal is never trustworthy).
Wired into the CLI / GUI / MCP generate paths, gated on clipSource ==
'model'. No model retrain needed; QTMESH_T2M_YAW180 override still works.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Model-clip conditioning (977e2c8) — fixes the last three field-reported artifacts on the v6.1 walk (which was otherwise good): walked backward, ballerina feet, arms too narrow. All three are retarget-layer consequences of the model's canonical convention, fixed in data-space (
Wired into CLI/GUI/MCP, gated on |
The 180°-about-Y premultiply was hand-expanded wrong — (x,y,z,w) → (-z,w,x,-y) — which is NOT a pure yaw: it flipped the arms to point UP (RarmY -0.99 → +0.99) and crumpled the pose into a hunched twist (the regression the user hit). Correct Hamilton product for yaw=(0,1,0,0): x'=z y'=w z'=-x w'=-y → (x,y,z,w) -> (z,w,-x,-y) Numeric check: posture Y components now UNCHANGED vs raw (a yaw can't alter vertical), hip-forward Z negates cleanly (+0.99 -> -0.99). Render-verified upright walk restored on both rigs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
#837) The facing flip negated restDir X/Z IN ADDITION to rotating the joint quats. The aim consumes ds = W_c · restDir, so rotating the quats alone gives ds = yaw·(old ds) — the body's directions rotate 180° as a RIGID unit, every limb keeping its relative bend. ALSO negating restDir made it a conjugation (yaw·W·yaw⁻¹·restDir), which flips hinge chirality: knees and elbows bent BACKWARD (the user's 'bends to the wrong side / walks sideways' report). Removing the restDir negation leaves a clean rigid yaw: forward-facing + correct forward knee flex. Verified: retargeted knee bend mean 19° (natural walk flex) vs the hyperextended pose before; front of body faces camera; stride intact on both the Mixamo rig and the chibi mouse. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Applies an arbitrary canonical clip JSON (as written by --dump-canonical) through the pure bind-referenced retarget — no model, no conditioning, no smooth/pin/arm-space. Enables a self-retarget round-trip (dump a rig's own clip -> apply-canonical back onto it -> re-dump -> compare) to measure per-bone retarget parity in isolation from the model. Baseline findings: - Rumba self-parity 0.92deg total (retarget core is faithful); minor asymmetric leg-amplitude loss (lfoot 3.7, lknee 2.5). - Mouse (chibi proportions) 5.07deg: arms severely compressed (relbow 23, amplitude 180->124) — the bind-referenced aim can't reproduce large rotations when target bone dirs diverge far from canonical. This is the measurable root of the 'tangled arms' look, now a gated target for retarget work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The per-role twist caps (#857) were added to damp the from-scratch model's noisy roll, but they also clamped legitimate large roll on authored/self-parity clips — collapsing real arm motion (harness: mouse elbow amp 180->124, self-parity total 5.1->3.1 with caps off). Split into kTwistCapModel (tight, model path) and kTwistCapOpen (uncapped), selected by a new applyMotionClip modelClip flag; the model call sites (CLI/GUI/MCP) pass clipSource=='model'. Self-parity: Rumba 0.92->0.75, mouse 5.07->3.06, no regression. Verified with --apply-canonical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Disciplined retarget pass — parity harness + verdict (025148f) Built a self-retarget parity harness (
The template path already produces a genuinely good walk on both a normal humanoid (Rumba) and the extreme chibi mouse — render-verified 3× each: forward-facing, upright, clean alternating stride, natural arms. That's the shipped default. Engineering verdict: the retarget + template path deliver the quality bar. The from-scratch model is the weak link and (per the parity data) not fixable to template quality by more retarget patching, a bigger model, or more of the same CMU data — so it stays experimental/opt-in. Template = default, and it's good. |
Model walks played BACKWARD (skeleton strides toward -Z while the mesh faces +Z) because the from-scratch model's canonical convention faces -Z and the retarget targets +Z-forward. Fix: yawFlipCanonicalClip rotates the model clip's world quats 180° about +Y — (x,y,z,w) -> (z,w,-x,-y) — leaving restDir untouched. This is a pure RIGID body turn: verified via the parity harness that facing (hip fwd Z) negates while knee bend (38.7°) and spine uprightness (0.94) are bit-preserved. Crucially NOT the earlier broken approach, which also negated restDir — that conjugates the aim and reverses knee/elbow hinge chirality (the 'knees bend backward' regression). restDir is left alone here. Gated model-path-only and XOR'd with the per-rig backward heuristic (flip iff clipSource=='model' && !yaw180, so a backward rig cancels it). Render-verified: model walk faces forward with correct forward knee bend on both the Mixamo rig and the chibi mouse. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This reverts commit da23151.
# Conflicts: # tests/CMakeLists.txt
- pinFeet: verify thigh/shin/foot keyframe TIMES align (not just count) before pinning — equal-count-different-time authored clips would write a correction computed at t onto a keyframe at t' (Codex P2). - MotionGenerator: guard the posture scorer on J>=22 — roles 11/17/21 would read OOB on a smaller-vocab model. - AnimationControlController: surface fp.error (footPinError) when foot-pinning fails instead of silently swallowing it. - prep-t2m-v5: per-item try/except so one bad clip/BVH doesn't abort the batch; guard empty vocab before np.stack. - train-t2m-flow-v5: torch.load(weights_only=True) on the resume ckpt. - upload-t2m-v5-model.sh: idempotent v4 backup — skip if the v4 rollback already exists (a re-run previously overwrote it with the v5 file). - test: null-check createEntity/getSkeleton in the collar twist test. Verified: clean build (0 errors) merged with master. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed review comments (dc6cc2f):
Note: several of the earlier facing/conditioning suggestions target code that was reverted — the model-facing conditioning attempts destabilized the pose (verified via the parity harness), so the shipped state uses the template path as default with the model as an experimental opt-in. Facing improvements are deferred to a follow-up PR per the plan. Clean build (0 errors) merged with master; CI running. |
…ults changes (#837) - TwistUnwrapKeepsDampedCollarContinuous: the per-role twist caps are now model-path-only (#837 review), so this test — which asserts the 240°→ capped-150°→×0.5-collar-gain = 75° behavior — must pass modelClip=true to exercise the capped path (was running uncapped and reaching 240°). - FootContact.ShortBlipsAreIgnored: the tighter default height band made the old data (2 low frames of 20) put the low-percentile ground level ON the lifted height, flagging the whole clip as contact. Rewrote the fixture to a realistic airborne foot with 3 isolated 1-frame dips — ground level lands near 0, each dip is < minFrames, no span registers. Verified against the exact detect algorithm. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The collar's MODEL twist cap is 30° (0.52 rad), not the old single 150° cap — clamp applies before the 0.5× collar gain, so a 240° source roll settles at 30 × 0.5 = 15°, not 75°. CI (unit-tests-linux) confirmed the retarget produces ~15°; the assertion + comment were stale from the pre-per-role-cap era. Continuity check (maxStep < 15°) unchanged — the unwrap still ramps smoothly to the cap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|



Closes #857, closes #856, closes #840, closes #858 — the remaining slices of epic #837, combined into one PR (supersedes the stacked #886/#887/#888).
1. Twist transport in the direction retarget (#857)
The bind-referenced retarget (PR #843) aimed bone directions but dropped roll — gesture clips read flat. The twist is now decomposed on the source and recomposed about the same pole the source decomposed about:
Recomposing from the target-bind pole (the issue's original formula) leaks roll into swing — measured +38% arm amplitude. With matched poles, per-frame relative motion is exactly the source's world delta conjugated into target axes. θ is unwrapped across frames (a ±180° pop would flip the roll once a gain ≠ 1 scales it), capped 150°; collars damped 0.5×.
Harness: Mixamo Walk self-parity 2.22° → 0.03° mean joint delta (gate ≤1.6°); spine amplitude restored (abdomen 2.3°→9.0°); Quaternius Knight cross-rig 6.68°→5.36°, renders upright. Controlled A/Bs pin a single-clip eval library — the runtime take-pick is quality²-weighted RANDOM, so uncontrolled comparisons are noisy.
2. Foot-contact detection + IK pinning (#856)
Retargeted clips skate/float feet on rigs whose proportions differ from the source. New
FootContactpure-data core (Ogre-free, 8 unit tests): leg-length-scaled contact detection (height band + horizontal-speed gate; a fast on-ground foot — skating — is correctly NOT a contact), analytic two-bonesolveKnee(keeps segment lengths + the pose's own bend plane), 0→1→0 edge blends.AnimationMerger::pinFeetruns manual FK over the tracks (the live skeleton is neverapply()'d), detects in the canonical frame, rewrites only thigh/shin/foot keyframes (foot keeps its original world orientation — no toe pop). Effectively idempotent.Surfaces: ON by default in generation — CLI
--no-foot-pin/ standalone--foot-pin --animation <n>; MCPfoot_pinarg +pin_feettool; GUI "Pin feet" checkbox.Matcher fix uncovered by this:
upperlegnow maps to the thigh role — Quaternius rigs (UpperLeg.L/LowerLeg.L) previously mapped BOTH leg bones to the knee.3. Flow-matching model on canonical v5 data (#840, #858)
prep-t2m-v5.pycanonicalizes every clip (corpus sidecar dumps + CMU BVH) into a rig-independent representation —Q'(f) = twist(θ, d(f)) · arc(D_c → d(f))against a fixed canonical T-pose — 17,286 windows / 18 actions (v4 trained on per-rig "convention mush", the measured cause of the CVAE's gentle-average output). The neutral-start gate is gone: model clips ride the direction retarget, which references the reference triple, not clip frame 0.train-t2m-flow-v5.py— 7.3M-param DiT (6D rotations, AdaLN-Zero), rectified flow + classifier-free guidance. The Euler sampler + CFG are unrolled inside the exported ONNX graph — the shippedMotionGeneratorcontract (tokens[1,V], seed[1,Z] → motion[1,T,220]) is unchanged; seed is the flattened noise tensor.MotionGenerator+ all three model call sites pass it through, so model clips ride the same bind-referenced retarget as templates (standing-pose shim retired to v4 fallback).applyMotionClip's restWorld presence check is norm-based now (identity counts as present).Hosted + verified: HF
motion/t2m.onnx+ vocab are the v5 CFG model (v4 preserved ast2m-v4.*for rollback; interface backward-compatible). E2E: fresh download →--generate run --model→ upright, coherent clip.Eval (honest): model walk/run/wave/dance all upright + temporally coherent (every v4 CVAE variant folded or tumbled); run/dance energetic and action-distinct with CFG; wave still gentler than the template take (CMU wave trials carry long idle stretches — data follow-up). Per the epic's gate, templates remain the default + automatic fallback; the model stays opt-in behind
--model.🤖 Generated with Claude Code
Summary by CodeRabbit